Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

ci(workflow): upgrade actions and compiler checks #533

Closed
wants to merge 2 commits into from
Closed

ci(workflow): upgrade actions and compiler checks #533

wants to merge 2 commits into from

Conversation

sambacha
Copy link
Contributor

This addresses #532

Action lint check:

https://rhysd.github.io/actionlint/#eJztl1tv0zAUx9/zKY66PWwP6QWhdgpimjYJ2ANiglY8Rm7iNKaunfmSUjG+O8dOlmVZQaC1EpXWl8b2uf1/x+pJBVnRCK6ugyCQIgoA1lItMy7Xccp0QUySR/DjJ+4XlvNY0VtLtYn8WufuG2CuiEhyqqsVQAgrog1VQZDSjFhu/ImyojLQOeU8gjnReRB8k3N/OreMp9U5GuoQawE7t8LYkBODKStXQ4tWGqsxKZDEMCn0AEtIltKai/J10JgIL+9aaEM4h3fSilRt6lOoA2TVbqj04P7RSMmTnDBxUY4a6zUzedSsAEqqNHOFCrbIDd90s94oJgxGVwt6b9t4Oxr1URjeH9anR3BJkuWaqFRDIlfYBDZnnJkNeIkNgCPMZHIKkqcICIhIQdC1e6wDgsyAkiQHbYtCKkNTWDEhVacaF6fe0V4k6IImLGMJMK2x3x1dV66KTmXerdUp92EZNoevyUafnD4WfteiiB0rrHl7fFLR8BcBmeglK8CFO23ZMtST5BJ6x5VXD+5goWgB4S30vhIlmFj03jgoouUFXafzc1y8v55+mF3Gn2bTm9m099j8OzMwam1lrNvb3zIY9s/6wwcEaQTlMD6Lhzumgs94eUHjNY18yoPDNOmPu5gm8XivmDDlAWJ6cpsme75NkwO8TeP+6FWX0zhu7+0BlEt6gKS2gNo3p/8M0xGUjIRMeUIMx5YbloataD1h+x2cHynRVtGn5jhgp05u89M/mrh57oP/GalDz1TsZsNosu21YBvTRAqj8H1H+41Bq5zB1cOzq6iP6Lszol3bPwq8JBjomSJjfOOju1fqStuh2i+JYsWzGxprH2a3aqvSdq51J72t9O6hxQ81/oVwU/83efkT0fh/tsJj0du8qk6UZem8stULu8f+1RRFoxUxBofSNmek5u6yswx+AQrwXZo=

Questions:

Shouldn't foundry test be using a defined seed value?
What about checking different EVM versions across SOLC versions?
Why not perform gas checks and persist gas snapshots as an artifact, so any regressions can be identified?

Simplify

Doing something like this would make adding EVM versions to the testing easier

jobs:
  build:
    runs-on: ubuntu-latest
    strategy:
      matrix:
     # These values match the names of test groups that will be run
        test-group:
          [
            0.8.0,
            0.7.6,
            0.7.0,
            0.6.12,
            0.6.2,
          ]
      fail-fast: false
    steps:
	# (...)
	# (...)
      - name: Check compatibility with ${{ matrix.test-group }}
        id: compat
        if: always()
        run: |
          output=$(forge build --skip test --use solc:/${{ matrix.test-group }}/)
          if echo "${output}" | grep -q "Warning"; then
              echo "${output}"  >> "$GITHUB_OUTPUT"
              exit 1
          fi

      - name: Pass or fail
        run: |
          if [[ ${{ steps.compat.outputs.output }} == 0 ]]; then exit 0; else exit 1; fi

@sambacha sambacha mentioned this pull request Mar 26, 2024
Merged
mds1
mds1 reviewed Apr 17, 2024
View reviewed changes
.github/workflows/sync.yml
@@ -8,14 +8,16 @@ on:
jobs:
sync-release-branch:
runs-on: ubuntu-latest
if: startsWith(github.event.release.tag_name, 'v1')
if: ${{ github.repository_owner == 'foundry-rs' && startsWith(github.event.release.tag_name, 'v1') }}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Presumably this change is to prevent the job running on forks?

@mds1
Copy link
Collaborator

mds1 commented Apr 17, 2024

Shouldn't foundry test be using a defined seed value?
What about checking different EVM versions across SOLC versions?
Why not perform gas checks and persist gas snapshots as an artifact, so any regressions can be identified?

These are all good questions. If we do address them let's do it in a separate PR, but TLDR is I think the current CI setup is sufficient coverage:

  1. Getting different random values in CI each time is ok here to get more coverage so a seed seems unnecessary
  2. The number of combinations here can get pretty big with the last few EVM versions, and the solidity team is good about keeping things backwards compatible (i.e. if our code compiles with cancun it will compile with shanghai and paris), so just checking the default one is ok
  3. We aren't gas sensitive in tests so this will mostly just add noise/friction to the contribution process IMO

mds1
mds1 reviewed Apr 17, 2024
View reviewed changes
.github/workflows/ci.yml
if echo "$output" | grep -q "Warning"; then
echo "$output"
echo "$output" >> "$GITHUB_OUTPUT"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need this?

@mds1 mds1 mentioned this pull request Apr 24, 2024
Merged
@mds1
Copy link
Collaborator

mds1 commented Apr 24, 2024

Superceded by #550 which is a bit smaller diff

@mds1 mds1 closed this Apr 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mds1 mds1 mds1 left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@sambacha @mds1